Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perform file downloads with netstack in the WG gateway probe #1329

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

durch
Copy link
Contributor

@durch durch commented Oct 18, 2024

This change is Reviewable

@durch durch changed the title Perform file downloads with netstack in the wg Perform file downloads with netstack in the WG gateway probe Oct 18, 2024
Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @benedettadavico, @mmsinclair, and @tommyv1987)


nym-vpn-core/crates/nym-gateway-probe/src/lib.rs line 221 at r1 (raw file):

        wg_outcome.can_register = true;

        if wg_outcome.can_register {

FYI this check seems irrelevant given that you wg_outcome.can_register = true just above but maybe there is a reason for that.


nym-vpn-core/crates/nym-gateway-probe/src/lib.rs line 238 at r1 (raw file):

                netstack_response.received_hosts as f32 / netstack_response.sent_hosts as f32;
            wg_outcome.ping_ips_performance =
                netstack_response.received_ips as f32 / netstack_response.sent_ips as f32;

FYI you might be ok with NaN so maybe no need to check for division by zero?

@pronebird
Copy link
Contributor

Clippy issue should go away if you rebase your branch. We've already fixed that in main

@pronebird
Copy link
Contributor

@durch bump

@tommyv1987
Copy link
Contributor

@durch I think you should incorporate these changes in to this branch... main...tommy-test as this has all the latest probing information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants